Skip to content

Fix for #774: made uiView link function manipulate the el passed into it... #921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

christopherthielen
Copy link
Contributor

Fix for #774
The element from compile step was being used during link function. However, the element had been removed from dom and replaced by a parent directive, so all link operations were occuring against a detached dom node. All tests pass.

@timkindberg
Copy link
Contributor

Pretty sure this is going to fix about 20 different issues we have going on right now (ui-view in ng-if, ui-view with ng-class, ui-view in custom transclue, etc) do you think I'm right?

@timkindberg
Copy link
Contributor

Would you be up for writing some tests that test each of those scenarios?

  • ui-view in custom transclude
  • ui-view in ng-if
  • ui-view with ng-class applied to it
  • ui-view in ng-repeat

These are all the actual symptoms that were happening because of this issue.

@timkindberg
Copy link
Contributor

Also the commit message would have to be: "fix(uiView): ui-view inside a directive that does ng-tranclude does not work, and other related issues when ui-view with ngIf, ngClass, ngRepeat, and ngTransclude"

@christopherthielen
Copy link
Contributor Author

I have never written any karma tests, but will give it a shot.

@timkindberg
Copy link
Contributor

Chris, you rock so much you have no idea. Thank you sincerely for pitching in. I hope it is rewarding for you!

@timkindberg
Copy link
Contributor

I just realized #858 may be trying to fix the same thing. Can you please review that PR and see if there's anything you need from it?

@christopherthielen
Copy link
Contributor Author

I just realized #858 may be trying to fix the same thing.

Doh, that PR looks like the exact same solution as mine

@meenie
Copy link
Contributor

meenie commented Mar 4, 2014

Would you be up for writing some tests that test each of those scenarios?

  • ui-view in custom transclude
  • ui-view in ng-if
  • ui-view with ng-class applied to it
  • ui-view in ng-repeat
    These are all the actual symptoms that were happening because of this issue.

@timkindberg - Just letting you know that I have specifically tested each one of those above and my pull request #858 has fixed each of them :). I'll see about writing some more tests with those in mind, but I can't today. Hopefully this weekend!

@meenie
Copy link
Contributor

meenie commented Mar 4, 2014

@timkindberg - Sorry, I forgot that I haven't pushed my latest changes that does enable ngRepeat. The uiView directive will need it's own isolate scope for it to work so you can do things like

<ui-view ng-repeat="view in views" name="{{view.name}}"></ui-view>

Instead of using an isolate scope, I could possibly just scope.$eval() the uiView and name attributes so that you can get the computed value.

@timkindberg
Copy link
Contributor

@meenie you rock. Would it be different if the ui-view was within an ng-repeat, instead of on same element?

@christopherthielen christopherthielen deleted the issue774 branch March 4, 2014 22:08
@meenie
Copy link
Contributor

meenie commented Mar 4, 2014

@timkindberg - No actually, it wouldn't matter where it was at inside an ngRepeat. I toyed around with using an isolate scope in uiView but that started to create a lot of issues. What I'm going to do is the attr.$observe() pattern on the name and uiView attributes so that it will interpolate the value passed in. We'll see how it goes :).

@timkindberg
Copy link
Contributor

Don't waste too much energy on that. I'd say using it with ngRepeat could be a different commit/feature if needed. We could consider it out of scope for this one if its too much of a pain.

@meenie
Copy link
Contributor

meenie commented Mar 4, 2014

I'll give it a quick try and see how it goes :).

@christopherthielen
Copy link
Contributor Author

#1324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants